Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NativeAOT: Disable AggressiveAttributeTrimming with ILLink #18545

Merged
merged 5 commits into from
Jul 17, 2023

Conversation

ivanpovazan
Copy link
Contributor

@ivanpovazan ivanpovazan commented Jul 10, 2023

In the current setup with NativeAOT, during app build we run both ILLink and ILCompiler in cascade.
When _AggressiveAttributeTrimming feature switch is set to true ILLink removes IsTrimmable attribute from assemblies, which means that when we are also running in TrimMode=partial (which translates to --defaultrooting ILC command-line arguments) ILC trimming is disabled completely.

This PR disables _AggressiveAttributeTrimming in the first pass ie during trimming by ILLink and enables it in the second trimming pass performed by ILCompiler.

Additionally, to workaround ILCompiler incompatibility with Microsoft.iOS (and friends) this platform assembly is explicitly rooted when passed to ILCompiler for trimming (this will be fixed once dotnet/runtime#86649 is resolved).

Estimated savings: This change reduces the size of the application bundle by 0,58Mb (or ~4,3% compared to the baseline)

MAUI ios app Base This PR diff (%)
SOD (Mb) 41,93 40,5 -3,4%
.ipa (Mb) 13,43 12,85 -4,3%

Fixes: #18479

@ivanpovazan
Copy link
Contributor Author

</RuntimeHostConfigurationOption>

<!-- Explicitly root the framework assembly due to NativeAOT trimming incompatibility tracked: https://github.com/dotnet/runtime/issues/86649 -->
<TrimmerRootAssembly Include="Microsoft.iOS" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<TrimmerRootAssembly Include="Microsoft.iOS" />
<TrimmerRootAssembly Include="$(_PlatformAssemblyName)" />

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name is different for each platform, so this needs to be adjusted. Aside from that, is there a longer description why is this necessary? I suppose with #18519 and #18529 this would no longer be necessary, correct?

Copy link
Contributor Author

@ivanpovazan ivanpovazan Jul 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the correction, it was an oversight. I corrected it.

I suppose with #18519 and #18529 this would no longer be necessary, correct?

If I am not mistaken, there is still an issue with ILC not knowing about [Preserve]. The issue I noted in the comment should track/document all these incompatibilities.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I am not mistaken, there is still an issue with ILC not knowing about [Preserve].

I am almost sure that [Preserve] in the platform assembly is used only for the constructors that would get rooted by the generated code in #18519.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@ivanpovazan
Copy link
Contributor Author

Test failures don't seem to be related.
This is now ready for review.

@ivanpovazan ivanpovazan marked this pull request as ready for review July 13, 2023 08:31
@ivanpovazan ivanpovazan changed the title [DRAFT] NativeAOT: Disable AggressiveAttributeTrimming with ILLink NativeAOT: Disable AggressiveAttributeTrimming with ILLink Jul 13, 2023
@ivanpovazan ivanpovazan requested a review from mauroa July 13, 2023 08:32
@ivanpovazan
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@vs-mobiletools-engineering-service2

This comment has been minimized.

@ivanpovazan
Copy link
Contributor Author

The tests failures do not seem to be related.
All the failures are due to test has timed out at 10s... and the failing tests in this run were not failing in the previous.

@SamMonoRT
Copy link

cc @dalexsoto @mandel-macaque - another one we are targeting for Preview 7

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [CI Build] Windows Integration Tests passed 💻

All Windows Integration Tests passed.

Pipeline on Agent
Hash: 9b31fbbfe71e800f989be7fcf25d80180feb8dfd [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent
Hash: [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11.5) passed 💻

All tests on macOS M1 - Mac Big Sur (11.5) passed.

Pipeline on Agent
Hash: [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

💻 [PR Build] Tests on macOS M1 - Mac Ventura (13.0) passed 💻

All tests on macOS M1 - Mac Ventura (13.0) passed.

Pipeline on Agent
Hash: [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

✅ API diff for current PR / commit

NET (empty diffs)
  • iOS: (empty diff detected)
  • tvOS: (empty diff detected)
  • MacCatalyst: (empty diff detected)
  • macOS: (empty diff detected)

❗ API diff vs stable (Breaking changes)

.NET (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
  • iOS: vsdrops gist (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
  • tvOS: vsdrops gist (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
  • MacCatalyst: vsdrops gist (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
  • macOS: vsdrops gist (:heavy_exclamation_mark: Breaking changes :heavy_exclamation_mark:)
  • Microsoft.iOS vs Microsoft.MacCatalyst: vsdrops gist

ℹ️ Generator diff

Generator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes)

Pipeline on Agent
Hash: 9b31fbbfe71e800f989be7fcf25d80180feb8dfd [PR build]

@vs-mobiletools-engineering-service2
Copy link
Collaborator

🚀 [CI Build] Test results 🚀

Test results

✅ All tests passed on VSTS: simulator tests.

🎉 All 92 tests passed 🎉

Tests counts

⚠️ bcl: No tests selected. Html Report (VSDrops) Download
✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests: All 1 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 4 tests passed. Html Report (VSDrops) Download
✅ framework: All 4 tests passed. Html Report (VSDrops) Download
✅ generator: All 1 tests passed. Html Report (VSDrops) Download
✅ interdependent_binding_projects: All 4 tests passed. Html Report (VSDrops) Download
⚠️ install_source: No tests selected. Html Report (VSDrops) Download
✅ introspection: All 4 tests passed. Html Report (VSDrops) Download
✅ linker: All 40 tests passed. Html Report (VSDrops) Download
⚠️ mac_binding_project: No tests selected. Html Report (VSDrops) Download
⚠️ mmp: No tests selected. Html Report (VSDrops) Download
⚠️ mononative: No tests selected. Html Report (VSDrops) Download
✅ monotouch: All 26 tests passed. Html Report (VSDrops) Download
✅ msbuild: All 2 tests passed. Html Report (VSDrops) Download
⚠️ mtouch: No tests selected. Html Report (VSDrops) Download
⚠️ xammac: No tests selected. Html Report (VSDrops) Download
✅ xcframework: All 4 tests passed. Html Report (VSDrops) Download
✅ xtro: All 1 tests passed. Html Report (VSDrops) Download

Pipeline on Agent
Hash: 9b31fbbfe71e800f989be7fcf25d80180feb8dfd [PR build]

@dalexsoto dalexsoto merged commit b68efbe into xamarin:net8.0 Jul 17, 2023
50 of 52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants